.NET: fix: RunState is sometimes Running when it should be Idle #2646
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lokitoth
changed the title
Copilot finished reviewing on behalf of
lokitoth
December 4, 2025 23:46
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where RunStatus was incorrectly set to Running when the workflow had no work to do. The fix ensures the Running state is only set when messages are actually being processed.
Key Changes:
- Moved
RunStatus.Runningassignment from before the message processing loop to inside the loop where actual work happens - Removed premature state changes that occurred after timeout waits, ensuring state reflects actual execution status
Comment on lines
+83
to
+84
| this._runStatus = RunStatus.Running; // Ensure we do not inappropriately signal Running status unless | ||
| // messages are being processed. |
There was a problem hiding this comment.
The RunStatus.Running state and state transitions during workflow execution lack test coverage. While tests verify Idle, PendingRequests, and Ended states, no tests verify that the Running state is correctly set during execution or that it correctly transitions to other states. Consider adding a test that verifies:
- The status is
Runningwhile messages are being processed - The status correctly transitions to
IdleorPendingRequestsafter processing completes - The status is not
Runningwhen there are no messages to process (the bug being fixed here)
There is a timeout-wait for messages in the inner loop driving the running of the workflow. When it gets cleared, an attempt to run a superstep is made. Currently we set the state to running as soon as the wait clears, even if we have no work to do, and clear it upon discovering this. The fix is to only set the Running state when actual Workflow execution is happening when running Super-Steps, and not until the loop confirms there is work to do. A broader-term fix would be to remove the Semaphore and timeout-wait in it, since it is working around the inability to atomicaly insert and release the Semaphore,
lokitoth
force-pushed
the
dev/dotnet_workflow/fix_unit_test_race_condition
branch
from
4c4e193 to
efbb023
Compare
lokitoth
had a problem deploying
to
integration
GitHub Actions
Failure
lokitoth
had a problem deploying
to
integration
GitHub Actions
Failure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
There is a timeout-wait for messages in the inner loop driving the running of the workflow. When it gets cleared, an attempt to run a superstep is made. Currently we set the state to running as soon as the wait clears, even if we have no work to do, and clear it upon discovering this.
This causes a sporadic UnitTest failure in Workflows.
Description
The fix is to only set the Running state when actual Workflow execution is happening when running Super-Steps, and not until the loop confirms there is work to do.
TODO: A broader-term fix would be to remove the Semaphore and timeout-wait in it, since it is working around the inability to atomically insert and release the Semaphore driving the wait. The timeout provides a race condition where the workflow was "ghosting" a Running status. See #2647
Contribution Checklist
Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.